feat(warn): detect global context on the server side #2983
Conversation
✅ Deploy Preview for pinia-official canceled.
|
packages/pinia/src/rootStore.ts
Outdated
| */ | ||
| export const getActivePinia = () => | ||
| (hasInjectionContext() && inject(piniaSymbol)) || activePinia | ||
| (hasInjectionContext() && inject(piniaSymbol)) || (import.meta.server ? throw new Error("Cannot get active pinia as it does not find context") : activePinia) |
There was a problem hiding this comment.
Thanks for the PR. This could be changed into a non breaking change by just showing an error and still returning the active pinia. The error should be in development only (like other warnings) and should not rely on import as it might not work in all scenarios
There was a problem hiding this comment.
I think failing fast is the better approach when the global activePinia is accessed on the server, because:
- failing immediately makes the problem obvious rather than silently continuing with potentially incorrect behavior.
- allowing it to continue by returning
activePiniacould lead to dangerous state sharing between requests, which is particularly problematic in server environments.
maybe making the message is more clear would help huge
There was a problem hiding this comment.
That would be a breaking change. Also, I do prefer the error message because it feels less frustrating to users
There was a problem hiding this comment.
I will leave the decision to you,
However, please keep in mind that the developers tend to ignore error messages, so they will make the same mistakes again and again, I would prefer it to fail that will indicate the mistake much faster.
|
@posva I removed throwing error, added |
posva
changed the title
|
Perfect @ivansky ! This would be great to have at least a warning. Would also have preferred the throw error but anything is better than nothing. |
|
Caution Review failedThe pull request is closed. WalkthroughAdds exported Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Env
participant API as getActivePinia()
participant Inject as Injection Context
participant Fallback as activePinia
participant Console as console.error / console.warn
Test->>API: call getActivePinia()
API->>Inject: read injected pinia
alt injected pinia exists
API->>Test: return injected pinia
else no injected pinia
alt not IS_CLIENT and __DEV__
API->>Console: emit dev-mode error (cross-request risk)
end
API->>Fallback: return activePinia (could be undefined)
API->>Test: return fallback result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #2983 +/- ##
==========================================
- Coverage 91.28% 91.16% -0.13%
==========================================
Files 18 18
Lines 1618 1629 +11
Branches 231 235 +4
==========================================
+ Hits 1477 1485 +8
- Misses 139 142 +3
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pinia/__tests__/vitest-mock-warn.ts (1)
19-121: Excellent refactoring!The generalization of the mock infrastructure to support both
warnanderrormethods is well-executed:
- Dynamic matcher name generation is correct for both methods
- All assertion logic (basic, last, times) properly adapted
- Unasserted log detection and error reporting maintained
- Eliminates code duplication while preserving functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/pinia/__tests__/ssr.spec.ts(2 hunks)packages/pinia/__tests__/vitest-mock-warn.ts(1 hunks)packages/pinia/src/rootStore.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/pinia/__tests__/ssr.spec.ts (2)
packages/pinia/__tests__/vitest-mock-warn.ts (2)
mockWarn(123-125)mockConsoleError(127-129)packages/pinia/src/rootStore.ts (2)
setActivePinia(35-35)getActivePinia(51-65)
packages/pinia/src/rootStore.ts (3)
packages/pinia/src/index.ts (2)
getActivePinia(4-4)Pinia(6-6)rollup.config.mjs (1)
__DEV__(166-171)packages/pinia/src/env.ts (1)
IS_CLIENT(1-1)
🔇 Additional comments (6)
packages/pinia/__tests__/ssr.spec.ts (2)
5-11: LGTM!The imports correctly reflect the newly exposed public API functions.
16-16: LGTM!The mock setup correctly initializes both warn and error tracking for the test suite, ensuring proper assertion of console output from
getActivePinia.Also applies to: 19-20
packages/pinia/src/rootStore.ts (2)
20-20: LGTM!The
IS_CLIENTimport is correctly used to detect server-side execution at line 55.
43-47: The review comment is incorrect.The
ImportMeta.serverdeclaration is actively used in the codebase (packages/nuxt/playground/pages/index.vue:12:if (import.meta.server)). This global type augmentation is necessary for TypeScript type safety when accessingimport.meta.server. Removing it would cause TypeScript compilation errors.Likely an incorrect or invalid review comment.
packages/pinia/__tests__/vitest-mock-warn.ts (2)
9-11: LGTM!The type definitions correctly extend the matcher API to support error assertions, mirroring the existing warn matchers.
123-129: LGTM!The exported functions provide a clean public API for test setup, properly delegating to the generic implementation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-project-automation
bot
moved this from 🧑💻 In progress
to ✅ Done
in Pinia Roadmap
3 participants
when store is accessed and injection context is not found it falls back to global active pinia object,
although it is safe for client, on the server side there are could be many requests with their own context,
falling back to global state leads to race conditions and undefined behaviour,
this should be fixed once and for all!
it is probably a breaking change but a good one, I haven't tested this locally, I appreciate any help to improve this PR in order to make it good looking
Summary by CodeRabbit
New Features
Tests
Improvements